Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: initial cut at support tiers for diag tools #21870

Closed
wants to merge 9 commits into from

Conversation

mhdawson
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Discussion on support tiers for diagnostic tools started at the Diagnostic summit earlier in the year. This specific text was discussed in the latest Diagnostics WG meeting and then tweaked based on the discussion. The goal would be to move tools to the target tier overtime as we manage to add the required testing.

@nodejs/diagnostics, @nodejs/release, @nodejs/tsc as this requires broad agreement on the approach/direction.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 18, 2018
tools. Each of the tools and APIs has been put them into one of
the following tiers.

* Tier 1 - Mmust always be working for all
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Mmust/Must

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Text LGTM. I have no strong opinion about the target tiers of the not yet classified tools.

@ofrobots
Copy link
Contributor

We had a discussion at the last add-on API meeting about whether the tiers of support should have something to do with whether N-API supports the APIs that that particular diagnostic tool needs.

For example, IMO, if CPU profiling is of tier-1 important to Node.js and the ecosystem, does it make sense that such diagnostics tools must use V8 APIs. Instead, should the criteria for tiers include whether N-API APIs are capable of supporting tools in that domain?

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits.

the runtime itself.

The Node.js project has assessed the tools and the APIs which support those
tools. Each of the tools and APIs has been put them into one of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is them unnecessary here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed


* If the tool fits into a key cateogry as listed below.
* Whether the tool is actively used by the community.
* The Availability of alternatives.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Availability -> availability?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

collaborative decision between Diagnostics WG and Release WG. Some of the
criteria considered might be:

* If the tool fits into a key cateogry as listed below.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cateogry -> category

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

* The availability of reliable test suite that can be integrated into our CI.
* The availability of maintainer or community collaborator who will help
resolve issues when there are CI failures.
* If the tool is maintained the Node.js foundation GitHub organization.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintained -> maintained by?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: foundation -> Foundation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


The current categories of tools/APIs that fall under these Tiers are:

* FDDC (F) - First failure data capture, Easy to consume initial diagnostic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FDDC -> FFDC?
Easy -> easy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

| Memory | node-heapdump | No | No | 2 |
| Memory | V8 heap profiler | No | Yes | 1 |
| Memory | V8 sampling heap profiler | No | Yes | 1 |
| AsyncFlow | Async Hooks(API) | ? | Yes | 1 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space before (API) here and below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

| Debugger | V8 Debug protocol(API) | No | Yes | 1 |
| Debugger | Command line Debug Client | ? | Yes | 1 |
| Debugger | Chrome Dev tools | ? | No | 3 |
| Debugger | Chakracore - time-travel | No | data source only | to early |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data -> Data (for consistency)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but was to early intended to be Too early?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes and yes

| Debugger | Chakracore - time-travel | No | data source only | to early |
| Tracing | trace_events(API) | No | Yes | 1 |
| Tracing | DTrace | No | Partial | 3 |
| Tracing | LTTng | No | Removed ? | N/A |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded space in Removed ??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

| Tracing | DTrace | No | Partial | 3 |
| Tracing | LTTng | No | Removed ? | N/A |
| Tracing | ETW | No | Partial | 3 |
| Profiling | V8 cpu profiler | No | Yes | 1 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cpu -> CPU?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include --prof as well? Is that included in V8 CPU profiler?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is the V8 cpu profiler ?

| Tracing | ETW | No | Partial | 3 |
| Profiling | V8 cpu profiler | No | Yes | 1 |
| Profiling | Linux perf | No | Partial | ? |
| Profiling | Windows xperf | No | ? | ? |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xperf -> Xperf?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

| Profiling | Linux perf | No | Partial | ? |
| Profiling | Windows xperf | No | ? | ? |
| Profiling | Ox | No | No | to early |
| Profiling | node-clinic | No | No | to early |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to -> too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

| Tracing | DTrace | No | Partial | 3 |
| Tracing | LTTng | No | Removed ? | N/A |
| Tracing | ETW | No | Partial | 3 |
| Profiling | V8 cpu profiler | No | Yes | 1 |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include --prof as well? Is that included in V8 CPU profiler?

in this tier it must have a good test suite and that test suite and a job
must exist in the Node.js CI so that it can be run as part of the release
process. No commit to master should break this tool/API if the next
release is within 1 month.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to defined what does it mean to "work". In some cases it's a true-false decision (llnode), but in others it is a matter of accuracy (Linux perf in 8.x for example). I assume you mean the latter, but I think it should be written down.

cc @yunong @mmarchini

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we'll require a test suite on Node.js CI for Tier 1 tools, maybe we could define "work" as "successfully running a CI for the tool's test suite"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, updated to try to clarify that.

@gireeshpunathil
Copy link
Member

  • tier1 tools should also be fully functional in all supported platforms. [ rationale: for a reported problem in any platform, I should have a tool to use reliably ]
  • tier1 tools should have minimum representation from all types of potential issues that those help diagnose. [ rationale: for any reported problem, I should have a tool to use reliably ]
  • ndoe-report: LGTM, we don't have an alternative in place that provide critical runtime state info
  • llnode: Also I feel llnode belongs to debugger instead of memory?
  • llnode: I would rate it tier1, given its unique capability to extract JS frame data at live debugging / core debugging sites
  • node-heapdump: I would rate it tier1 unless we have other alternatives for heapdump.
  • linux perf: I would rate tier1, again unique capability to map CPU info with JS entities
  • Everything else: LGTM

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put a strong requirement for all Tier 1 and Tier 2 tools to have documentation within the Node.js organization, in the form of a guide or proper docs.

I would also require all tools to be Open Source.

| Profiling | V8 cpu profiler | No | Yes | 1 |
| Profiling | Linux perf | No | Partial | ? |
| Profiling | Windows xperf | No | ? | ? |
| Profiling | Ox | No | No | to early |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would classify 0x being "Tier 4" for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok will make that the target, but I'd like to handle tool promotion from unclassified to specified tiers with separate PRs.

| Profiling | Ox | No | No | to early |
| Profiling | node-clinic | No | No | to early |
| F/P/T | appmetrics | No | No | ? |
| M/T | eBPF tracing tool | No | No | ? |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we convert this list to links to the various tools?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I'd prefer to do that in a separate PR. I will commit to doing that once this PR lands.

in this tier it must have a good test suite and that test suite and a job
must exist in the Node.js CI so that it can be run as part of the release
process. No commit to master should break this tool/API if the next
release is within 1 month.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove the condition. Current releases usually happen at least once a month.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better: keep the condition but change it to "if the next major release is within 1 month"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@targos
Copy link
Member

targos commented Jul 19, 2018

It doesn't have to be in this PR but I would like to have a link pointing to the documentation or repo for each tool.

@mmarchini
Copy link
Contributor

I would put a strong requirement for all Tier 1 and Tier 2 tools to have documentation within the Node.js organization, in the form of a guide or proper docs.

Strongly agree.

llnode: I would rate it tier1, given its unique capability to extract JS frame data at live debugging / core debugging sites

I would like to rate it Tier 1 as well, but there's only a handful of people working on the project right now and it essentially breaks at every V8 upgrade. Besides, some optimizations on V8 are making it harder to fix those breakages, and llnode use a best-effort approach to diagnose the application, which makes its result not as reliable as we'd expect from a Tier 1 tool. We started some discussion on this topic at nodejs/diagnostics#202.

Linux perf: I would rate tier1, again unique capability to map CPU info with JS entities

We could classify Linux perf and other system profilers (DTrace, eBPF, ETW) which provide similar features as Tier 1, but what should we do if V8 revamps its compiler again in the future without adding support for those tools (like what happenend with Turbofan)? I think this is an important question for all tools that might break with V8 updates.

Should we include --prof as well? Is that included in V8 CPU profiler?

I think we should, even though --prof is not officially supported by V8 (there are other tools in the same situation in the list, such as Linux perf and llnode).

| Debugger | Chrome Dev tools | ? | No | 3 |
| Debugger | Chakracore - time-travel | No | data source only | to early |
| Tracing | trace_events(API) | No | Yes | 1 |
| Tracing | DTrace | No | Partial | 3 |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • DTrace is also used as a profiling tool and is widely used to generate FlameGraphs
  • Systemtap should also be included since its situation is similar to DTrace (for tracing): we have a file specifying Systemtap tracepoints (https://github.com/nodejs/node/blob/master/src/node.stp). I would add it as Tracing | Systemtap | No | Partial | ?

@mhdawson
Copy link
Member Author

Thanks for all the feedback so far. I'll take a cut at incorporating it on my travels to SF for Node Summit this weekend.

@vsemozhetbyt
Copy link
Contributor

A new one, but seems worth to add?
https://github.com/GoogleChromeLabs/ndb

@joyeecheung
Copy link
Member

joyeecheung commented Jul 22, 2018

I am -1 on rating llnode tier 1, at least for the moment. My reasoning was described in nodejs/diagnostics#202 (comment) . llnode is just a plugin of lldb (correction: it can also be an addon linked to lldb now that the JS API landed). Unlike V8 or chromium, we don't have effective contact with the lldb project and it does not even support core file debugging that well, which is what llnode primarily used for (the situation may have been improved since that post but I don't feel a significant difference from day-to-day use of lldb). We should try to improve the situation, yes, but when talking about support and expectations, we should not make this a hard dependency of Node.js's release process.

* Tier 1 - Mmust always be working for all
Current and LTS Node.js releases. A release will not be shipped if the test
suite for the tool/API is not green. To be considered for inclusion
in this tier it must have a good test suite and that test suite and a job
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we include entries similar to those required by citgm:

The maintainers of the module remain responsive when there are problems
The module must be actively used by the community
The module must be heavily depended on

Given that tier-one tools block our commits and releases.

@addaleax
Copy link
Member

Given that this document is basically a list of things officially supported by Node.js, i.e. we’re picking winners here (which we have very much tried to avoid so far), I think there should be a clear policy for tool authors to have their tools entered into this list before we merge this?

@mhdawson
Copy link
Member Author

@joyeecheung it was listed as tier 1 only as a longer term target, not what it will be. But happy to have it as something lower as an long term target as well.

@mhdawson
Copy link
Member Author

mhdawson commented Jul 30, 2018

Back in the office this week, so will try to do an update tomorrow.

@addaleax everything is starting at level "Unclassified", but I do get your point that we need to be careful about making"winners". On the other hand there are some cases were we need some additional support to even have one tool that is good enough for the ecosystem. I'd be happy to drop out some of the newer ones, but I think the approach was to make the "Unclassified" the largest set to show, if nothing else, that the project was aware of them and that they were not going to get into the higher tiers.

@addaleax
Copy link
Member

@mhdawson I’m not worried about the particular amount of packages on this list, but I’d really like to see a policy that can guide package maintainers into having their own tools accepted into this document.

@mhdawson
Copy link
Member Author

mhdawson commented Aug 1, 2018

@addaleax, thanks for the clarification. I'll think about that and try to incorporate some more info on that front when I update tomorrow.

@mhdawson
Copy link
Member Author

mhdawson commented Aug 2, 2018

Failed to get to this today and I'm away Friday/Monday will be a priority for next week.

@gireeshpunathil
Copy link
Member

@joyeecheung - I understand your reasoning - llnode may be difficult to be maintained as tier1 or its tier1 status may adversely affect releases.

But that does not necessarily affect its criticality to the postmortem debugging. Please note, we don't have an alternative! It may be true that we don't have too many dump debugging scenarios - Node's prominent problem symptom seems to be exceptions. It may also be true that most common dump debugging are covered by JS stack + heap snapshot (I am not very sure about this). But how about the composite stack that shows complete call sequences? How about the ability to access v8 data structures and JS objects? I am concerned that if problems such as memory corruption, crash in the compiled code and crash in v8 APIs happen, the native debuggers will have severely short insights to provide. All these 3 scenarios require that we want the C++ and JS execution space logically connected, and examine those together.

My dealings with crash scenarios (largely nodejs/help repo) primarily had users in the development phase, so it was easy to play around with instrumented code and do trial and error - much more easier than dump debugging. When large scale production systems crash, (there may not be an identical test / staging system, the issue may be intermittent or even one-off) I fear we won't have the leeway for experimenting like that, and will have to resort to extract everything from the single core file we have, and llnode looks the most natural choice for that, and hence my proposal.

@joyeecheung
Copy link
Member

joyeecheung commented Aug 3, 2018

@gireeshpunathil I feel like this is getting a little off-topic, since in this PR llnode is unclassified and should be classified in future PRs. But FWIW, criticality is only one of the many factors for deciding support. When llnode is broken, the fix rarely lies in Node.js itself. Having the releases of project A blocked because project B breaks project C does not really help with anything, at best you may have chances forcing project B to support project C because they support project A, but there is little that project A can do other than politicking.

criteria considered might be:

* If the tool fits into a key cateogry as listed below.
* Whether the tool is actively used by the community.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you define "the community"? Is this...

  • Open source developers?
  • The Node.js core collaborators?
  • Developers deploying to FaaS infrastructure?
  • Massive enterprise deployments?

We tend to use "the community" as a catch-all where I think we may be more well served by saying "the ecosystem" – but even then, we need to be explicit when we're codifying expectations around what that means.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to the ecosystem. I don't think its any one of the above at the exclusion of the others. It is that its actively used by enough of the overall ecosystem which includes all of the categories you listed.

@mhdawson
Copy link
Member Author

mhdawson commented Aug 7, 2018

Believe I have updated for the comments so far. Please take another look: @mcollina @bnb @Trott @Alhadis @targos @mmarchini @joyeecheung @vsemozhetbyt @jasnell @BridgeAR

suite for the tool/API is not green. To be considered for inclusion
in this tier it must have a good test suite and that test suite and a job
must exist in the Node.js CI so that it can be run as part of the release
process. No commit to master should break this tool/API if the next
Copy link
Member

@joyeecheung joyeecheung Aug 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should be more specific about what No commit to master should break this tool/API means? We probably will not run the tests of these tools for every PR to master, so this will be hard to enforce as-is. We may either:

  1. Run the tests of these tools with master every day/week and revert any recent commit that breaks the tests
  2. Or, acknowledge that we may have commits on master that break the tools without being noticed (because we may not run the tooling tests for those PRs), and limit this to no commit to the current and LTS release branches should break..., which means we will run the tooling tests in the release process and exclude commits that break the tests (optionally, run the tests when someone suspects a PR may break the tools, i.g. similar to how CITGM works). This would be more practical.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with 2, given you running for every commit is problematic. In addition to the release process we should still aim to run them nightly if possible so that we get earlier warning if possible.

@BridgeAR
Copy link
Member

@BridgeAR
Copy link
Member

@nodejs/build-infra PTAL. There is something weird going on with the CI

@Trott
Copy link
Member

Trott commented Aug 13, 2018

@nodejs/build-infra PTAL. There is something weird going on with the CI

Looks like the directory needs a cleaning, maybe?

HEAD detached at 0c60ab9a55
Untracked files:
  (use "git add <file>..." to include in what will be committed)

	tools/doc/node_modules/.bin/
	tools/doc/node_modules/bail/
	tools/doc/node_modules/boolbase/
	tools/doc/node_modules/camelcase/
	tools/doc/node_modules/ccount/
	tools/doc/node_modules/character-entities-html4/
	tools/doc/node_modules/character-entities-legacy/
	tools/doc/node_modules/character-entities/
	tools/doc/node_modules/character-reference-invalid/
	tools/doc/node_modules/collapse-white-space/
	tools/doc/node_modules/comma-separated-tokens/
	tools/doc/node_modules/css-selector-parser/
	tools/doc/node_modules/debug/
	tools/doc/node_modules/define-properties/
	tools/doc/node_modules/detab/
	tools/doc/node_modules/extend/
	tools/doc/node_modules/foreach/
	tools/doc/node_modules/function-bind/
	tools/doc/node_modules/has/
	tools/doc/node_modules/hast-to-hyperscript/
	tools/doc/node_modules/hast-util-from-parse5/
	tools/doc/node_modules/hast-util-is-element/
	tools/doc/node_modules/hast-util-parse-selector/
	tools/doc/node_modules/hast-util-raw/
	tools/doc/node_modules/hast-util-sanitize/
	tools/doc/node_modules/hast-util-to-html/
	tools/doc/node_modules/hast-util-to-parse5/
	tools/doc/node_modules/hast-util-whitespace/
	tools/doc/node_modules/hastscript/
	tools/doc/node_modules/html-void-elements/
	tools/doc/node_modules/inherits/
	tools/doc/node_modules/is-alphabetical/
	tools/doc/node_modules/is-alphanumerical/
	tools/doc/node_modules/is-buffer/
	tools/doc/node_modules/is-decimal/
	tools/doc/node_modules/is-hexadecimal/
	tools/doc/node_modules/is-nan/
	tools/doc/node_modules/is-plain-obj/
	tools/doc/node_modules/is-whitespace-character/
	tools/doc/node_modules/is-word-character/
	tools/doc/node_modules/kebab-case/
	tools/doc/node_modules/lodash.iteratee/
	tools/doc/node_modules/longest-streak/
	tools/doc/node_modules/mapz/
	tools/doc/node_modules/markdown-escapes/
	tools/doc/node_modules/markdown-table/
	tools/doc/node_modules/mdast-util-definitions/
	tools/doc/node_modules/mdast-util-to-hast/
	tools/doc/node_modules/mdurl/
	tools/doc/node_modules/ms/
	tools/doc/node_modules/nth-check/
	tools/doc/node_modules/object-assign/
	tools/doc/node_modules/object-keys/
	tools/doc/node_modules/once/
	tools/doc/node_modules/parse-entities/
	tools/doc/node_modules/parse5/
	tools/doc/node_modules/property-information/
	tools/doc/node_modules/rehype-raw/
	tools/doc/node_modules/rehype-stringify/
	tools/doc/node_modules/remark-html/
	tools/doc/node_modules/remark-parse/
	tools/doc/node_modules/remark-rehype/
	tools/doc/node_modules/remark-stringify/
	tools/doc/node_modules/remark/
	tools/doc/node_modules/repeat-string/
	tools/doc/node_modules/replace-ext/
	tools/doc/node_modules/space-separated-tokens/
	tools/doc/node_modules/state-toggle/
	tools/doc/node_modules/stringify-entities/
	tools/doc/node_modules/to-vfile/
	tools/doc/node_modules/trim-lines/
	tools/doc/node_modules/trim-trailing-lines/
	tools/doc/node_modules/trim/
	tools/doc/node_modules/trough/
	tools/doc/node_modules/unherit/
	tools/doc/node_modules/unified/
	tools/doc/node_modules/unist-builder/
	tools/doc/node_modules/unist-util-find/
	tools/doc/node_modules/unist-util-generated/
	tools/doc/node_modules/unist-util-is/
	tools/doc/node_modules/unist-util-position/
	tools/doc/node_modules/unist-util-remove-position/
	tools/doc/node_modules/unist-util-select/
	tools/doc/node_modules/unist-util-stringify-position/
	tools/doc/node_modules/unist-util-visit-parents/
	tools/doc/node_modules/unist-util-visit/
	tools/doc/node_modules/vfile-location/
	tools/doc/node_modules/vfile-message/
	tools/doc/node_modules/vfile/
	tools/doc/node_modules/web-namespaces/
	tools/doc/node_modules/wrappy/
	tools/doc/node_modules/x-is-array/
	tools/doc/node_modules/x-is-string/
	tools/doc/node_modules/xtend/
	tools/doc/node_modules/zwitch/

nothing added to commit but untracked files present (use "git add" to track)
+ git rev-parse HEAD
0c60ab9a55d2b51698ba65ff6c342f434fa33980
+ git rev-parse origin/master
5442c28b651a79c2269bf2b931e81cd553171656
+ '[' -n origin/master ']'
+ git rebase --committer-date-is-author-date origin/master
First, rewinding head to replay your work on top of it...
Applying: doc: initial cut at support tiers for diag tools
Applying: squash: partially address first set of comments
Applying: squash: address more comments
Applying: squash: address additional comments.
Applying: squash: address additional comments
Applying: squash: address rest of comments
Applying: squash: address comments
Applying: squash: fix linting issues from comments
Applying: squash: address comments
+ '[' -n 0c60ab9a55d2b51698ba65ff6c342f434fa33980 ']'
+ check_sha1=0c60ab9a55d2b51698ba65ff6c342f434fa33980
++ git rev-parse HEAD
+ head_sha1=43bdd12e130db75061672ae2ec6557e5503f430b
+ '[' 43bdd12e130db75061672ae2ec6557e5503f430b '!=' 0c60ab9a55d2b51698ba65ff6c342f434fa33980 ']'
+ exit 1

@Trott
Copy link
Member

Trott commented Aug 13, 2018

More recent run shows this:

gmake: *** [Makefile:1147: lint-js-ci] Error 1
+ cat test-eslint.tap
+ grep -v '^ok\|^TAP version 13\|^1\.\.'
+ sed '/^/\s*$/d'
sed: 1: "/^/\s*$/d": invalid command code \

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 25, 2018
@mmarchini
Copy link
Contributor

@mhdawson
Copy link
Member Author

@mhdawson
Copy link
Member Author

mhdawson commented Aug 27, 2018

@mhdawson
Copy link
Member Author

CI green going to land.

@mhdawson
Copy link
Member Author

Landed in c241f4a

@mhdawson mhdawson closed this Aug 28, 2018
mhdawson added a commit that referenced this pull request Aug 28, 2018
PR-URL: #21870
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
addaleax pushed a commit that referenced this pull request Aug 28, 2018
PR-URL: #21870
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
PR-URL: #21870
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
targos pushed a commit that referenced this pull request Sep 6, 2018
PR-URL: #21870
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
@mhdawson mhdawson deleted the diag-tiers branch September 30, 2019 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.